Skip to content

feat: SettingsDialog#746

Open
nextchamp-saqib wants to merge 3 commits into
frappe:mainfrom
nextchamp-saqib:feat-2356
Open

feat: SettingsDialog#746
nextchamp-saqib wants to merge 3 commits into
frappe:mainfrom
nextchamp-saqib:feat-2356

Conversation

@nextchamp-saqib

@nextchamp-saqib nextchamp-saqib commented May 31, 2026

Copy link
Copy Markdown
Member
CleanShot 2026-05-31 at 12 25 21@2x

Coverage: 56.09% (+0.06% vs main)

@netchampfaris

netchampfaris commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

@nextchamp-saqib Add tests and stories with good examples. Do a review with v1 guidelines and philosophy.

@nextchamp-saqib

Copy link
Copy Markdown
Member Author

@netchampfaris the docs preview is skipped, can it be ran for this PR for review?

@netchampfaris

Copy link
Copy Markdown
Contributor

/barista review

@barista-for-frappe

barista-for-frappe Bot commented Jun 3, 2026

Copy link
Copy Markdown

Re-reviewing per @netchampfaris — full v1 + philosophy pass.

Concerns — visibility binds to the wrong v-model, plus a likely async bug. Tests and stories are in good shape.

  • SettingsDialog.vue:280 / types.ts:615 — visibility is exposed via unnamed v-model (modelValue + update:modelValue). For overlays the canonical name is open via v-model:open (P2; CONTEXT.md: "for overlay components, modelValue is not the visibility — visibility is open"). Dialog already does this — Dialog.vue:2 uses v-model:open, and Dialog/types.ts:54-59 documents open as canonical with modelValue kept only for back-compat. A brand-new component shouldn't inherit the legacy binding. Switch to defineModel<boolean>('open') and emit update:open.
  • SettingsDialog.vue:289activeTab is seeded once from allItems.value[0] at setup. If sections load async (empty at first mount — common for a settings dialog), activeTab stays undefined and never picks up the first item once data arrives. Worth a watch/computed fallback so the first item selects when sections change.

Minor:

  • SettingsDialog.vue:261 — the #tab-content slot name isn't in the shared vocab (P6). The content region is the dialog's main content; #default reads more naturally and still carries { tab }. Not blocking, but worth considering before freeze.

SettingsTab/SettingsSection extending the Sidebar types, reusing DialogSize, and pulling the hljs theme into a shared file are all the right moves.

barista · claude-opus-4-8 · 8.0k in / 8.5k out · 1850k cached · 113s · $0.859

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants